Add filterableAttributes syntax API to settings #730
Conversation
WalkthroughAdds mixed-syntax support for Meilisearch v1.14 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Index as Index API
participant Server as Meilisearch
rect rgba(135,206,250,0.06)
Note over Client,Index: Update filterable attributes (mixed syntax)
Client->>Index: set_filterable_attributes_advanced([ "author", { attributePatterns:[...], features:{...} } ])
Index->>Server: POST /settings/filterable-attributes (body: Vec<FilterableAttribute>)
Server-->>Index: TaskInfo
Index-->>Client: Result<TaskInfo>
end
rect rgba(220,220,220,0.04)
Note over Client,Index: Backward-compatible string usage
Client->>Index: set_filterable_attributes(["author","genre"])
Index->>Index: Wrap strings -> FilterableAttribute::Attribute(...)
Index->>Server: POST /settings/filterable-attributes (body: Vec<FilterableAttribute>)
Server-->>Index: TaskInfo
Index-->>Client: Result<TaskInfo>
end
rect rgba(144,238,144,0.06)
Note over Client,Index: Retrieve mixed syntax
Client->>Index: get_filterable_attributes_advanced()
Index->>Server: GET /settings/filterable-attributes
Server-->>Index: Vec<FilterableAttribute> (mixed)
Index-->>Client: Result<Vec<FilterableAttribute>>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/settings.rs (1)
1614-1633: Consider more flexible signature forset_filterable_attributes_advanced.The current signature
impl IntoIterator<Item = FilterableAttribute>requires owned values, which prevents passing borrowed slices like&[FilterableAttribute]. This differs from other setter methods (e.g.,set_stop_wordsat line 1497) that useItem = impl AsRef<str>to accept both owned and borrowed values.While the current design is simpler and works correctly, consider whether accepting borrowed values would improve ergonomics:
pub async fn set_filterable_attributes_advanced( &self, filterable_attributes: &[FilterableAttribute], ) -> Result<TaskInfo, Error> { self.client .http_client .request::<(), Vec<FilterableAttribute>, TaskInfo>( &format!(...), Method::Put { query: (), body: filterable_attributes.to_vec(), }, 202, ) .await }This would allow callers to retain ownership of their configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.code-samples.meilisearch.yaml(1 hunks)src/documents.rs(1 hunks)src/settings.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/settings.rs (1)
src/indexes.rs (13)
client(186-188)client(232-234)client(320-322)client(376-378)client(428-430)client(473-475)client(522-525)client(556-558)client(634-636)client(700-702)client(970-972)client(1038-1040)client(1089-1091)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (5)
.code-samples.meilisearch.yaml (1)
549-567: LGTM: Imports and mixed-syntax demonstration are correct.The new types are properly imported and the example correctly demonstrates both legacy syntax (using
.into()for string conversion) and the new settings object syntax.src/documents.rs (1)
706-713: LGTM: Test correctly updated for new FilterableAttribute type.The test properly imports and uses the new
FilterableAttributeenum, wrapping attribute names in theAttributevariant. This correctly reflects the changes toSettings::filterable_attributesfromOption<Vec<String>>toOption<Vec<FilterableAttribute>>.src/settings.rs (3)
67-114: LGTM: Well-structured types for mixed-syntax filterable attributes.The new types properly model Meilisearch v1.14's advanced filterable attributes syntax:
- Hierarchical structure with
FilterFeatureModes→FilterFeatures→FilterableAttributesSettings- Untagged enum allows seamless JSON serialization of mixed plain strings and settings objects
Fromimplementations provide ergonomic string-to-attribute conversion- Proper camelCase JSON mapping via serde annotations
244-399: LGTM: Backward-compatible API design with advanced method.The changes maintain backward compatibility:
- Existing
with_filterable_attributesaccepts strings and converts internally- New
with_filterable_attributes_advancedaccepts mixedFilterableAttributeitems- Clear documentation distinguishes the two approaches
This design allows gradual adoption of the new syntax while preserving existing code.
767-799: LGTM: Dual getter methods provide both legacy and advanced access.The addition of
get_filterable_attributes_advancedalongside the existingget_filterable_attributesenables:
- Legacy code continues using
Vec<String>return type- New code can retrieve mixed-syntax
Vec<FilterableAttribute>Both methods query the same endpoint but deserialize to different types, maintaining backward compatibility.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
+ Coverage 86.18% 86.42% +0.23%
==========================================
Files 20 20
Lines 6263 6395 +132
==========================================
+ Hits 5398 5527 +129
- Misses 865 868 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kumarUjjawal sorry, but can you resolve the git conflicts please? |
Yeah, I will resolve it soon. |
|
@curquiza what's this issue with code coverage? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/settings.rs (2)
67-93: Consider enhancing documentation for new public types.The new public types
FilterFeatureModes,FilterFeatures, andFilterableAttributesSettingshave minimal documentation. Since these are part of the public API for the v1.14 filterable attributes feature, consider adding:
- Brief descriptions of each field's purpose
- Example usage showing how to construct these types
- Links to relevant Meilisearch documentation
This will improve discoverability and reduce confusion for SDK users.
795-813: Consider adding usage guidance in documentation.The implementation is correct, but the doc comment could clarify when to use
get_filterable_attributes_advancedversusget_filterable_attributes. Consider adding:
- A note that this method returns the full mixed-syntax representation
- When users should prefer this over the simpler string-based method
- An example showing the expected return format
.code-samples.meilisearch.yaml (1)
573-592: LGTM! Clear demonstration of mixed-syntax feature.The code sample effectively demonstrates the new v1.14 filterable attributes feature:
- Imports all necessary types
- Shows legacy syntax via
"author".into()- Shows advanced syntax with
FilterableAttribute::Settings- Includes realistic feature flags (facetSearch, equality filter only)
Consider adding a brief comment above line 581 explaining that arrays can contain any mix of strings and settings objects, to reinforce the "mixed syntax" concept for documentation readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.code-samples.meilisearch.yaml(1 hunks)src/documents.rs(1 hunks)src/settings.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/documents.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/settings.rs (2)
src/client.rs (2)
new(54-66)index(424-426)src/request.rs (1)
body(49-56)
🔇 Additional comments (6)
src/settings.rs (5)
95-114: LGTM! Well-designed enum with convenient conversions.The untagged
FilterableAttributeenum correctly models the mixed-syntax API. The structural difference between a plainStringandFilterableAttributesSettingsensures serde will deserialize correctly without ambiguity. TheFromimplementations provide ergonomic conversions.
387-413: LGTM! Excellent backward compatibility strategy.The refactored
with_filterable_attributesmaintains API compatibility by wrapping strings inFilterableAttribute::Attribute, while the newwith_filterable_attributes_advancedprovides direct access to the mixed-syntax feature. The inline comment on line 392 clearly documents the intent.
1604-1647: LGTM! Consistent backward-compatible implementation.Both methods correctly handle the new
FilterableAttributetype. The updatedset_filterable_attributesmaintains API compatibility by wrapping string inputs, whileset_filterable_attributes_advancedprovides direct access to the mixed-syntax feature. The approach is consistent with the builder methods.
2921-2921: LGTM! Necessary test imports.The additional
to_stringimport is required for the new test that validates request body serialization.
2923-3027: LGTM! Comprehensive test coverage for advanced filterable attributes.The three new tests effectively cover:
- Builder pattern with mixed-syntax attributes
- Request body serialization (ensuring the JSON format matches expectations)
- Response deserialization (ensuring mixed responses are parsed correctly)
The use of mockito for HTTP-level testing is appropriate and allows precise control over request/response validation.
.code-samples.meilisearch.yaml (1)
594-598: LGTM! Correct method invocation.The call to
set_filterable_attributes_advanced(filterable_attributes)correctly passes ownership of the vector, matching the method signature that expectsimpl IntoIterator<Item = FilterableAttribute>. The previous issue with passing a reference has been resolved.
| /// | ||
| /// Supports both plain attribute names and settings objects. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub filterable_attributes: Option<Vec<String>>, | ||
| pub filterable_attributes: Option<Vec<FilterableAttribute>>, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify migration path for breaking change to filterable_attributes field.
The type change from Option<Vec<String>> to Option<Vec<FilterableAttribute>> is a breaking change for code that directly accesses Settings.filterable_attributes. While the builder methods maintain backward compatibility, users who construct Settings via struct literals or access the field directly will need to update their code.
Consider:
- Documenting this breaking change in the changelog/migration guide
- Verifying that the most common usage patterns (via builder methods) remain unaffected
Run the following script to identify potential usage patterns in the codebase:
🏁 Script executed:
#!/bin/bash
# Find direct field access or struct construction patterns
rg -nP --type=rust -C3 'Settings\s*\{.*filterable_attributes' -g '!src/settings.rs'
rg -nP --type=rust -C3 '\.filterable_attributes\s*=' -g '!src/settings.rs'Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Broader search for filterable_attributes usage (any pattern)
echo "=== General usage of filterable_attributes ==="
rg -nP --type=rust 'filterable_attributes' -g '!src/settings.rs' | head -20
echo -e "\n=== Builder method patterns ==="
rg -nP --type=rust 'with_filterable_attributes|set_filterable_attributes' -C2
echo -e "\n=== Check for FilterableAttribute type definition ==="
rg -nP --type=rust 'struct FilterableAttribute|enum FilterableAttribute|type FilterableAttribute'
echo -e "\n=== Check settings.rs for builder methods ==="
rg -nP --type=rust -A5 'fn.*filterable_attributes' src/settings.rs
Length of output: 11128
Breaking change confirmed: direct field access to filterable_attributes will require updates.
Direct field access exists in the codebase (e.g., documents.rs lines 754, 760 access filterable_attributes directly). Code that constructs Settings via struct literals or directly accesses the field must be updated to work with Vec<FilterableAttribute> instead of Vec<String>.
The builder methods (with_filterable_attributes(), set_filterable_attributes()) maintain backward compatibility and continue accepting strings, so most typical usage patterns are unaffected. However, any code iterating over the field or relying on String methods will break. Consider documenting this as a breaking change in the migration guide.
2 participants
Pull Request
Related issue
Fixes #660
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Backward Compatibility
Documentation / Tests
✏️ Tip: You can customize this high-level summary in your review settings.